[WIP] move lib/include dir search into @rules in native_toolchain.py #6271

cosmicexplorer · 2018-07-29T21:35:45Z

WIP because I'm waiting on #6374 to use the datatype default value helpers.

Problem

Searching for lib and include directories to provide to our compilers and linkers is still pretty brittle. Part of this is that while we provide include dirs through e.g. the CPATH environment variable, clang and gcc will still prefer "system" include dirs they find themselves over ours, and this leads to e.g. breakages across untested linux platforms.

Solution

Add -nostdinc, -nobuiltininc (this is osx-only), and -nostdinc++ where possible to our compiler command lines in native_toolchain.py to stop them from searching for include directories by themselves.
Remove LibcDev and ParseSearchDirs to move al(most al)l toolchain configuration into native_toolchain.py, where we search for lib dirs as before (by parsing the output of a compiler executable invocation), and also introduce include path searching in the same way, but invoking the compiler with a different command.
Change library_dirs from Executable to be instead named runtime_library_dirs.

Result

The logic from LibcDev and ParseSearchDirs is now spread into several @rules in native_toolchain.py, and that functionality is now much easier to access. We now separately search for include directories, which allows us to turn on -nostdinc/etc and have deeper control over what resources we pull in for compilation.

now blocking on pantsbuild#6271

CMLivingston · 2018-07-30T21:06:19Z

src/python/pants/backend/native/config/environment.py

    }

+  def as_execute_process_request(self, more_args=None):
+    argv = [self.exe_filename] + self.extra_args + (more_args or [])


more_args can be a tuple here to eliminate the or

Only sort-of. If you want to be defensive, this is appropriate. The caller can explicitly pass None.

CMLivingston · 2018-07-30T21:13:24Z

src/python/pants/backend/native/subsystems/binaries/llvm.py

@@ -90,7 +90,7 @@ def linker(self, platform):
      path_entries=self.path_entries,
      exe_filename=platform.resolve_platform_specific(
        self._PLATFORM_SPECIFIC_LINKER_NAME),
-      library_dirs=[],
+      runtime_library_dirs=[],
      linking_library_dirs=[],


Im slightly confused on the difference here between runtime_library_dirs and linking_library_dirs. Is the latter for static libs?

Directories containing shared libraries that must be on the runtime library search path. (this is LD_LIBRARY_PATH or DYLD_LIBRARY_PATH) vs Directories to search for libraries needed at link time. (this is LIBRARY_PATH, it's for both static and shared libs).

CMLivingston · 2018-07-30T21:26:17Z

src/python/pants/backend/native/subsystems/native_toolchain.py

+def parse_known_lib_dirs(compiler_search_request):
+  # FIXME: convert this to using `copy()` when #6269 is merged.
+  cmplr = compiler_search_request.compiler
+  print_search_dirs_exe = type(cmplr)(path_entries=cmplr.path_entries,


These should be tuples to conform to other compiler instantiations in this changeset. I may be missing something, but its good to have consistency. Also, not sure how I feel about this construction using type. Might be cleaner to have this as a class method on the compiler class, however I don't have any explicit reason to change this beyond Python conventions/readability.

+1 to classmethod over constructor here.

I mentioned that this should be using a .copy() method with an explicit comment -- that has now been implemented and removes all of this.

If by "tuples" you mean positional arguments, that's much more brittle to future changes in the fields of the datatype than explicitly specifying keyword arguments in a .copy() call.

CMLivingston · 2018-07-30T21:29:51Z

src/python/pants/backend/native/subsystems/native_toolchain.py

+  #   c_compiler=c_compiler.copy(include_dirs=compiler_search_output.include_dirs.dirs),
+  #   c_linker=c_linker.copy(linking_library_dirs=compiler_search_output.lib_dirs.dirs)))
+  resolved_c_toolchain = CToolchain(
+    c_compiler=CCompiler(path_entries=c_compiler.path_entries,


tuples here as well?

What is meant by "tuples" here?

CMLivingston

Thanks for doing this! This is a huge improvement to the native toolchain and makes the compile environment way easier to understand. I left a few comments/questions but no blockers. Feel free to take or leave the suggestions.

Also, I would like to see one more approval by a developer who is more familiar with rules and snapshots. I am still wrapping my head around these concepts, however details of rules aside, these changes look good to me.

jsirois · 2018-08-01T18:00:13Z

src/python/pants/backend/native/config/environment.py

    }

+  def as_execute_process_request(self, more_args=None):
+    argv = [self.exe_filename] + self.extra_args + (more_args or [])


Only sort-of. If you want to be defensive, this is appropriate. The caller can explicitly pass None.

jsirois · 2018-08-01T18:03:06Z

src/python/pants/backend/native/config/environment.py

+]), LinkerMixin):
+
+  @property
+  def with_tupled_collections(self):


The 3 with_tupled_collections are unused - kill. Additionally, with_* reads like a verb and not like a noun, so either @property is not appropriate or the naming is suboptimal.

jsirois · 2018-08-01T18:07:33Z

src/python/pants/backend/native/subsystems/native_toolchain.py

+def parse_known_lib_dirs(compiler_search_request):
+  # FIXME: convert this to using `copy()` when #6269 is merged.
+  cmplr = compiler_search_request.compiler
+  print_search_dirs_exe = type(cmplr)(path_entries=cmplr.path_entries,


+1 to classmethod over constructor here.

…tables

Eric-Arellano · 2020-05-17T22:21:35Z

Closing due to being stale, such as no longer using datatype.

cosmicexplorer changed the title ~~[WIP] move lib/include dir search into @rules in native_toolchain.py and remove ParseSearchDirs and LibcDev~~ [WIP] move lib/include dir search into @rules in native_toolchain.py Jul 29, 2018

cosmicexplorer requested review from CMLivingston, jsirois and stuhood July 29, 2018 21:36

cosmicexplorer added a commit to cosmicexplorer/pants that referenced this pull request Jul 30, 2018

expand the flag passthrough logic more, and add a FIXME

be304b8

now blocking on pantsbuild#6271

cosmicexplorer added a commit to cosmicexplorer/pants that referenced this pull request Jul 30, 2018

expand the flag passthrough logic more, and add a FIXME

2d360ee

now blocking on pantsbuild#6271

cosmicexplorer mentioned this pull request Jul 30, 2018

[WIP] Pants distutils extensions to directly use our toolchain environment #6273

Closed

CMLivingston reviewed Jul 30, 2018

View reviewed changes

CMLivingston approved these changes Jul 30, 2018

View reviewed changes

jsirois reviewed Aug 1, 2018

View reviewed changes

cosmicexplorer added 7 commits September 1, 2018 02:53

refactor invocation of Executables and lib/include dir search

06ac05f

change library_dirs -> runtime_library_dirs

971b85b

remove some more copy calls until pantsbuild#6269 is merged

15bb9e4

refactor argument and environment generation for native backend execu…

2fefc56

…tables

add process execution rules to python dist testing

3d28419

remove CFLAGS/CXXFLAGS split in python_dist compilation, adding a FIXME

a886016

add -nobuiltininc flag on osx

7614765

cosmicexplorer force-pushed the refactor-lib-include-search-native branch from fbdac6e to e582161 Compare September 1, 2018 10:03

cosmicexplorer added 2 commits September 1, 2018 03:03

make the native backend tests pass

e582161

complete many FIXMEs/TODOs

62c9505

Eric-Arellano closed this May 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] move lib/include dir search into @rules in native_toolchain.py #6271

[WIP] move lib/include dir search into @rules in native_toolchain.py #6271

cosmicexplorer commented Jul 29, 2018 •

edited

Loading

CMLivingston Jul 30, 2018

jsirois Aug 1, 2018

CMLivingston Jul 30, 2018

cosmicexplorer Sep 1, 2018 •

edited

Loading

CMLivingston Jul 30, 2018 •

edited

Loading

jsirois Aug 1, 2018

cosmicexplorer Sep 1, 2018

cosmicexplorer Sep 1, 2018

CMLivingston Jul 30, 2018

cosmicexplorer Sep 1, 2018

CMLivingston left a comment

jsirois Aug 1, 2018

jsirois Aug 1, 2018

jsirois Aug 1, 2018

Eric-Arellano commented May 17, 2020

[WIP] move lib/include dir search into @rules in native_toolchain.py #6271

[WIP] move lib/include dir search into @rules in native_toolchain.py #6271

Conversation

cosmicexplorer commented Jul 29, 2018 • edited Loading

Problem

Solution

Result

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosmicexplorer Sep 1, 2018 • edited Loading

Choose a reason for hiding this comment

CMLivingston Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CMLivingston left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano commented May 17, 2020

cosmicexplorer commented Jul 29, 2018 •

edited

Loading

cosmicexplorer Sep 1, 2018 •

edited

Loading

CMLivingston Jul 30, 2018 •

edited

Loading